Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Reworked guard comparison. #2015

Closed
wants to merge 1 commit into from
Closed

Reworked guard comparison. #2015

wants to merge 1 commit into from

Conversation

seven-phases-max
Copy link
Member

Changes:

[1]. Fixes #1180 case (4).
[2]. Adds support for list/array values comparison (#2003 test (4)).

[3]. Fixes #2009, #1725. I've simply "disabled" greater/less operators for anything except "number vs. number" and "quoted string vs. quoted string" comparisons. The main show-stopper for other cases ("unquoted string vs. unquoted string vs. anything else" in particular) is that after all it comes to the toCSS-based comparison and thus brings all those "nice" side effects of the alphabetical >/< ops with confusing (in the "unquoted context") results like:

abc > "abc"  // true
6em > "7"    // true
1px < ~"1"   // true
1   < ~"1 "  // true

It's not really a big deal to put more consistency for such comparisons but the code quickly becomes too bloated (handling all those Keyword, Anonymous, escaped Quoted and their combinations) for supporting of likely never existing use-cases (assuming this rationale). In fact I was even thinking of disabling the greater/less stuff for quoted strings too (as it feels quite weird to support quoted strings more than the unquoted) but I decided to keep it for a sort of "demo" purposes (i.e. just to keep corresponding functions and code structure set up for further possible changes (if any)).

@lukeapage
Copy link
Member

Shall we merge into 2.0 since its kind of a breaking change (which shouldn't effect anyone) ?

@seven-phases-max
Copy link
Member Author

Technically, yes, it is quite breaking since now we get false where it was true. But most of those true were invalid as in #2009 (e.g. 2 < ~"1" -> true) so this likely can break only already broken code. So I'm for merging but you know I'm much more tolerate to a "minor breaking" :)

@rjgotten
Copy link
Contributor

Why not adopt a type-converting comparison similar to the abstract relational comparison algorithm from ECMAScript?

Given a comparison a < b:

  1. If both operands' evaluated values are quoted strings, always perform a string comparison.

  2. Try to parse the evaluated value of both operand expressions into Dimension instances.

    a. If both operands could be succesfully interpreted as dimensions, perform comparison based on the following rules:

    • If neither dimension has a unit, then compare their values and return the result.
    • If the dimensions have the same unit, then compare their values and return the result.
    • If the dimensions have different units that are compatible (e.g. s and ms), then normalize them to the same unit, compare their values and return the result.
    • If the dimensions have different units that are incompatible, always return false.
    • If one dimension has a unit, the other does not have a unit and strict-units is enabled, then always return false.
    • If one dimension has a unit, the other does not have a unit and strict-units is disabled, then compare their values as if both dimensions did not have a unit and return the result.

    b. If one or more of the operands could not be interpreted as a dimension, convert both to quoted strings and perform a string comparison.

@lukeapage
Copy link
Member

Why would we? We want to keep things simple if we can.

@matthew-dean
Copy link
Member

What @lukeapage said. Unlike ECMAScript, style vars should not (typically) be so complex or unknown that a developer would need to coerce a type for comparison.

@seven-phases-max seven-phases-max mentioned this pull request Jun 29, 2014
Closed
@seven-phases-max seven-phases-max deleted the reworked-guard-comparison branch September 1, 2014 21:00
@seven-phases-max seven-phases-max mentioned this pull request Sep 3, 2014
Closed
@seven-phases-max
Copy link
Member Author

Oops, I mistakenly deleted the branch thinking the PR has been merged already. Restoring it back and rerererequesting... -> #2117.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Weird results of comparison of non-numeric types with <, > and similar operators. Guards, numbers and units
4 participants
@seven-phases-max @lukeapage @rjgotten @matthew-dean